Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support esm #5585

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Conversation

thepassle
Copy link

@thepassle thepassle commented Jun 20, 2024

📑 Summary

This change allows mermaid to be imported (import mermaid from 'mermaid') as native esm in the browser without requiring any buildtools/buildstep/transpilation/commonjs magic

Resolves #5584 #4320

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

📋 Tasks

Make sure you

Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 01b7dd6
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/6674250b4fdaf5000707f3fc
😎 Deploy Preview https://deploy-preview-5585--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Enhancement New feature or request label Jun 20, 2024
@@ -326,7 +326,7 @@ describe('when formatting urls', function () {
expect(result).toEqual(url);

result = utils.formatUrl(url, { securityLevel: 'strict' });
expect(result).toEqual('about:blank');
expect(result).toEqual('javascript:alert(%22test%22)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this turns out to be an issue, we can also consider using a different url sanitization library, ill leave that up to the maintainers to decide :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit worried that this might cause XSS attacks (although, it might be impossible without using URL unsafe characters like ", as they'll get converted to %22).

If somebody makes a malicious mermaid diagram with a malicious javascript:xxxx link, hosts it on example.com, and convinces somebody to click it, I believe the JavaScript will have access to the user's data on example.com

However, it's possible that https://github.com/cure53/DOMPurify will handle that for us, though, even if it isn't handled by the URI sanitizer library. I'll let @knsv chime in on it, since he probably knows more!

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 26.66667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 5.73%. Comparing base (fe9fbd8) to head (01b7dd6).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5585   +/-   ##
=======================================
  Coverage     5.73%   5.73%           
=======================================
  Files          278     277    -1     
  Lines        41999   41988   -11     
  Branches       490     515   +25     
=======================================
  Hits          2409    2409           
+ Misses       39590   39579   -11     
Flag Coverage Δ
unit 5.73% <26.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/mermaid/src/logger.ts 44.44% <100.00%> (ø)
packages/mermaid/src/diagrams/gantt/ganttDb.js 77.47% <66.66%> (ø)
...ckages/mermaid/src/diagrams/gantt/ganttRenderer.js 0.00% <0.00%> (ø)
packages/mermaid/src/utils.ts 41.60% <50.00%> (ø)
packages/mermaid/src/diagrams/c4/svgDraw.js 0.00% <0.00%> (ø)
...kages/mermaid/src/diagrams/common/svgDrawCommon.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/sequence/svgDraw.js 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@sidharthv96 sidharthv96 requested a review from knsv June 20, 2024 13:03
Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an issue with the new library.

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea behind this, and I definitely also want to switch to a fully ESM library, but the main issue I see is that dayjs/esm doesn't fully support ESM, so some bundlers/libraries can't handle it, see iamkun/dayjs#1765

They were working on a dayjs v2 that was supposed to properly ESM, but maintainance of dayjs has slowed down, so 🤷, I don't know when it will be fixed.

@@ -326,7 +326,7 @@ describe('when formatting urls', function () {
expect(result).toEqual(url);

result = utils.formatUrl(url, { securityLevel: 'strict' });
expect(result).toEqual('about:blank');
expect(result).toEqual('javascript:alert(%22test%22)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit worried that this might cause XSS attacks (although, it might be impossible without using URL unsafe characters like ", as they'll get converted to %22).

If somebody makes a malicious mermaid diagram with a malicious javascript:xxxx link, hosts it on example.com, and convinces somebody to click it, I believe the JavaScript will have access to the user's data on example.com

However, it's possible that https://github.com/cure53/DOMPurify will handle that for us, though, even if it isn't handled by the URI sanitizer library. I'll let @knsv chime in on it, since he probably knows more!

@@ -2,7 +2,7 @@
/* eslint-disable @typescript-eslint/no-unused-vars */
/* eslint-disable @typescript-eslint/no-empty-function */
/* eslint-disable no-console */
import dayjs from 'dayjs';
import dayjs from 'dayjs/esm/index.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I think dayjs's ESM support is still a bit lackluster: iamkun/dayjs#1765

We've previously tried to use dayjs/esm, but had to revert it due to dayjs's ESM support being non-standard, and so many users faced issues with it: #4438

@sidharthv96 sidharthv96 marked this pull request as draft June 23, 2024 07:14
@sidharthv96
Copy link
Member

The dayjs issue is a blocker. Converting to draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mermaid Library issue
3 participants